Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 26 user defined params #44

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Conversation

rtimms
Copy link
Collaborator

@rtimms rtimms commented Aug 31, 2023

Allows user-defined parameters to be added using the field ["Parameterisation"]["User-defined"].

Fixes #26

@rtimms rtimms changed the base branch from main to develop August 31, 2023 21:20
@ejfdickinson
Copy link
Collaborator

Do we want the string alias to be User-defined verbatim, with hyphen and lowercase "defined"? I think that's most consistent with the usage elsewhere in the schema.

@rtimms
Copy link
Collaborator Author

rtimms commented Sep 25, 2023

Apologies, my description is inconsistent with the code I wrote! In the code, the alias is "User defined". I've updated the description to match the code.

EDIT: I also need to add the hyphen. Will update.

@ejfdickinson
Copy link
Collaborator

Yeah, I think the hyphen is required - it's pedantic to say so but that is kind of the point!

Copy link
Collaborator

@ejfdickinson ejfdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one very minor issue I noted - correct in CHANGELOG.md to lowercase for:

"User-defined"

@ejfdickinson
Copy link
Collaborator

@rtimms One issue that occurred to me that I haven't been able to test, but perhaps you could.

I wasn't sure from your latest type-fixing commit - do we allow an arbitrary nested structure in the JSON under "User-defined". It feels like we should - that is, we shouldn't care if there is extra JSON hierarchy within this zone.

Would treating all dict as InterpolatedTable break this?

@rtimms
Copy link
Collaborator Author

rtimms commented Oct 5, 2023

We do not currently allow this kind of structure. Everything in "User-defined" must be of the type FloatFunctionTable. I'm reaching the limit of my pydantic knowledge now, but what you are suggesting should be possible. I'll give it a try, but may need to call in reinforcements!

@ejfdickinson
Copy link
Collaborator

@rtimms Happy to save that for a future update if necessary. Besides anything else, we would need to more carefully define what is and is not an InterpolatedTable.

@rtimms
Copy link
Collaborator Author

rtimms commented Oct 19, 2023

@ejfdickinson, let's leave the careful definition of what is and is not an InterpolatedTable for a future issue. Otherwise, does this PR look ok to you?

@ejfdickinson
Copy link
Collaborator

@rtimms Yes, if the tests are passing including the example I generated, that's fine. Can you make a new issue for supporting arbitrary JSON hierarchy under User-defined?

@rtimms
Copy link
Collaborator Author

rtimms commented Oct 19, 2023

Thanks, new issue is here #46

Can you hit "approve" so this can be merged?

@ejfdickinson ejfdickinson self-requested a review October 19, 2023 09:48
@rtimms rtimms merged commit 79ecbc2 into develop Oct 19, 2023
12 checks passed
@rtimms rtimms deleted the issue-26-user-defined-params branch October 19, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow "miscellaneous" category
2 participants